Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "all fields" execution mode to simple_query_string query #21341

Merged
merged 1 commit into from
Nov 9, 2016

Conversation

dakrone
Copy link
Member

@dakrone dakrone commented Nov 4, 2016

This commit introduces a new execution mode for the
simple_query_string query, which is intended down the road to be a
replacement for the current _all field.

It now does auto-field-expansion and auto-leniency when the following criteria
are ALL met:

The _all field is disabled
No default_field has been set in the index settings
No fields are specified in the request

Additionally, a user can force the "all-like" execution by setting the
all_fields parameter to true.

When executing in all field mode, the simple_query_string query will
look at all the fields in the mapping that are not metafields and can be
searched, and automatically expand the list of fields that are going to
be queried.

Relates to #20925, which is the query_string version of this work.
This is basically the same behavior, but for the simple_query_string
query.

Relates to #19784

Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left two minors, LGTM otherwise

SearchResponse resp = client().prepareSearch("test").setQuery(
simpleQueryStringQuery("127.0.0.2 \"2015/09/02\"")
.field("f_ip") // Usually this would mean we wouldn't search "all" fields
.useAllFields(true)) // ... unless explicitly requested
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just throw an exception in that case (like fields does) ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! I actually missed this and the exception throwing was only validating the xcontent parsing, so things like the transport client weren't tripping it. I've changed that to validate it always.

// suggest doesn't match
// geo_point doesn't match
// geo_shape doesn't match
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you'll want to have a test mixing numbers and text ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I've added another test for mixing the types (other tests in the suite do this as well)

@jpountz
Copy link
Contributor

jpountz commented Nov 7, 2016

The fact that we need to implement this feature on multiple query parsers makes me wonder whether this could be done at the mapping level, ie the _all field type would check whether it is enabled, and return a disjunction over all existing (and supported) fields when it is disabled. Maybe this way we could make it transparent to query parsers? One issue I can think is that we would need to add a phraseQuery method to MappedFieldType so that fields could customize how phrase queries are handled (_all would need to return a disjunction over phrase queries for each matching field I think?).

@clintongormley
Copy link
Contributor

@jpountz i thought of something similar, but do we really want this mode to be used in (eg) term queries?

@dakrone
Copy link
Member Author

dakrone commented Nov 7, 2016

@jpountz when I talked to Clint about it (basically creating a "meta-field"), I think the all_fields parameter is better. We have to implement it on multiple query parsers, however, the implementation is slightly different for each, and there are only 3 that are really being targeted (query_string, simple_query_string, and multi_match). I think implementing it at the mapping level is going to be just as or more complex.

@jpountz
Copy link
Contributor

jpountz commented Nov 7, 2016

do we really want this mode to be used in (eg) term queries?

Good question. We already do something similar with the _index field, which you can query with a term query even though it is not stored explicitly. If we really want to have a distinction, we could make sure to use the already existing MappedFieldType.queryStringTermQuery method in query parsers, and termQuery in term queries, which I suspect was the initial goal of having those two methods.

I think implementing it at the mapping level is going to be just as or more complex.

Fair enough, I just wanted to make sure we considered this option.

@dakrone
Copy link
Member Author

dakrone commented Nov 7, 2016

retest this please

1 similar comment
@dakrone
Copy link
Member Author

dakrone commented Nov 8, 2016

retest this please

@dakrone
Copy link
Member Author

dakrone commented Nov 8, 2016

retest this please but don't fail pulling the builds this time :-/

@dakrone dakrone force-pushed the sqs-all-field-mode branch from 34806a7 to a1b6bb5 Compare November 9, 2016 17:38
This commit introduces a new execution mode for the
`simple_query_string` query, which is intended down the road to be a
replacement for the current _all field.

It now does auto-field-expansion and auto-leniency when the following criteria
are ALL met:

    The _all field is disabled
    No default_field has been set in the index settings
    No fields are specified in the request

Additionally, a user can force the "all-like" execution by setting the
all_fields parameter to true.

When executing in all field mode, the `simple_query_string` query will
look at all the fields in the mapping that are not metafields and can be
searched, and automatically expand the list of fields that are going to
be queried.

Relates to elastic#20925, which is the `query_string` version of this work.
This is basically the same behavior, but for the `simple_query_string`
query.

Relates to elastic#19784
@dakrone dakrone force-pushed the sqs-all-field-mode branch from a1b6bb5 to 7420fd0 Compare November 9, 2016 17:38
@dakrone dakrone merged commit 7420fd0 into elastic:master Nov 9, 2016
dakrone added a commit to dakrone/elasticsearch that referenced this pull request Nov 22, 2016
As part of elastic#20925 and elastic#21341 we added an "all-fields" mode to the
`query_string` and `simple_query_string`. This would expand the query to
all fields and automatically set `lenient` to true.

However, we should still allow a user to override the `lenient` flag to
whichever value they desire, should they add it in the request. This
commit does that.
dakrone added a commit to dakrone/elasticsearch that referenced this pull request Jan 11, 2017
This change disables the _all meta field by default.

Now that we have the "all-fields" method of query execution, we can save both
indexing time and disk space by disabling it.

_all can no longer be configured for indices created after 6.0.

Relates to elastic#20925 and elastic#21341
Resolves elastic#19784
@dakrone dakrone deleted the sqs-all-field-mode branch January 23, 2017 17:23
@clintongormley clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Query DSL labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>feature :Search/Search Search-related issues that do not fall into other categories v5.1.1 v6.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants